chore: unified cookie storage - WPB-23404#4596
Merged
Merged
Conversation
This is dead code apart from in tests
This reverts commit fa6dc9b.
Account name should be unique per server. That is the assumption we are making when storing in the keychain so no point doing something different for the cache. As far as I understand it was only there for legacy migrations
This is a Swift version of ZMPersistentCookieStorage
This file isn't used # Conflicts: # WireNetwork/Sources/WireNetwork/Assembly.swift
|
johnxnguyen
approved these changes
May 4, 2026
Collaborator
johnxnguyen
left a comment
johnxnguyen
left a comment
There was a problem hiding this comment.
Looks good! Left a couple comments.
# Conflicts: # wire-ios-sync-engine/Source/SessionManager/SessionManager.swift # wire-ios-sync-engine/Source/UserSession/ZMUserSession/ZMUserSession.swift # wire-ios-sync-engine/WireSyncEngine.xcodeproj/project.pbxproj # wire-ios/WireUITests/FederationTests.swift # wire-ios/WireUITests/Helper/WireUITestCase.swift # wire-ios/WireUITests/MultiBackendSupportTests.swift
netbe
reviewed
May 6, 2026
netbe
approved these changes
May 6, 2026
Collaborator
netbe
left a comment
netbe
left a comment
There was a problem hiding this comment.
Looks good, left some suggestions, also check sonarQube comments some are relevant can be fixed by any AI tool
Contributor
Author
|
"DO NOT MERGE" label is applied as there is a crash when sending gifs (due to a nil response) |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
My apologies. This PR has turned into something much bigger than I intended 🙏.
Currently the app has two versions of CookieStorage - the legacy obj-c ZMPersistentCookieStorage & the new CookieStorage. There are some issues with this such as the legacy version using a cache and the new one not... E.g. if the cookie is updated via the new cookie storage, this may not be reflected in the legacy cookie due to it's cache.
This PR attempts to unify the cookie storage mechanisms and make some other safety improvements to try to make everything work well in a concurrent and multiprocess environment. Specifically it:
CookieStoragemaking it a non actor and removing async methods so that it can be used from non async contexts. It's access it protected by a lock to ensure that storing, fetching & deleting are generally atomic operations.CookieStorageact on all users not a single user. Eg. a call tofetchCookies()is nowfetchCookies(userID: UUID). The reasoning for this is that 1. in some cases cookie storage is not cleanly tied to a user session, 2. There are currently some static methods on LegacyCookieStorage (e.g. testing that any cookie exists). These methods should move toCookieStorageeventually and it would be better to make them non static to allow for injection etc.CookieStorageand changes how it works. We now store an epoch as metadata to Keychain items when they are . When adding to the cache we also store the epoch. When fetching from the cache we compare an items epoch to that in the Keychain to determine if the cached item is still valid. This works across processes meaning that we can now uses caching in app extensions and an cookie update coming from an app extension would now not break the cache in the main app (I'm not sure this happens in practice but believe it could happen).ZMPersistentCookieStorageintoLegacyCookieStoragewhich now wrapsCookieStorageand is a much simpler object. Interestingly I started this refactor by using Claude to convertZMPersistentCookieStorageand its tests from obj-c to swift. This worked well.NSHTTPCookieAcceptPolicyAlways.This PR also fixes: https://wearezeta.atlassian.net/browse/WPB-24720
@johnxnguyen @netbe I accidently merged as dependent PR - 84eafe5 - into this before this was reviewed. This contains UI tests and has been independently reviewed. Feel free to skip over reviewing the UI test code.
Testing
General regression test logging in & out of multiple accounts, and making use of app extensions.
Checklist
[WPB-XXX].